api: support to query whether pd has loaded region #8749

lhy1024 · 2024-10-28T09:04:44Z

What problem does this PR solve?

Issue Number: Close #8748

What is changed and how does it work?

Check List

Tests

Unit test
Integration test

Release note

None.

Signed-off-by: lhy1024 <[email protected]>

ti-chi-bot · 2024-10-28T09:04:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from lhy1024, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2024-10-28T09:11:58Z

Codecov Report

Attention: Patch coverage is 77.77778% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.63%. Comparing base (0402e15) to head (df4ac32).
Report is 30 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8749      +/-   ##
==========================================
- Coverage   70.10%   69.63%   -0.48%     
==========================================
  Files         517      516       -1     
  Lines       79983    79706     -277     
==========================================
- Hits        56076    55500     -576     
- Misses      20339    20694     +355     
+ Partials     3568     3512      -56

Flag	Coverage Δ
unittests	`69.63% <77.77%> (-0.48%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

rleungx · 2024-10-29T08:55:54Z

pkg/storage/storage.go

+	regionLoadedFromDefault bool
+	regionLoadedFromStorage bool


Why do we need to distinguish these two kinds?

I want to avoid problems after ps.useRegionStorage switch

How about using string?

regionLoadedFrom: "", "etcd", "leveldb"

I don't think so, we just have these two ways now

I prefer just using a single field to store this info like:

type regionSource int const ( unloaded regionSource = iota etcd leveldb )

Signed-off-by: lhy1024 <[email protected]>

lhy1024 · 2024-10-30T04:24:24Z

server/api/status.go

@@ -39,11 +40,13 @@ func newStatusHandler(svr *server.Server, rd *render.Render) *statusHandler {
 // @Success  200  {object}  versioninfo.Status
 // @Router   /status [get]


I am not sure whether it is suitable. ref #8748 (comment)

How about /member or /health?

/members return all member info according etcd.ListEtcdMembers
/health is to query the status of each node through the /ping interface to return information about all the nodes, if we don't add a new api, we won't be able to get the result of whether the load region is complete or not.

status is more like some fixed information to me, at least for the current definition.

put it under /status, which is appropriate from the name, but currently status returns compiled information.

Yes, as I said in the issue.

rleungx · 2024-10-30T07:19:19Z

pkg/versioninfo/versioninfo.go

+	Version          string `json:"version"`
+	GitHash          string `json:"git_hash"`
+	StartTimestamp   int64  `json:"start_timestamp"`
+	AreRegionsLoaded bool   `json:"are_regions_loaded"`


prefer to use loaded

Loaded bool json:"loaded"

?

Signed-off-by: lhy1024 <[email protected]>

okJiang · 2024-11-06T03:55:19Z

pkg/versioninfo/versioninfo.go

@@ -31,6 +31,7 @@ type Status struct {
 	Version        string `json:"version"`
 	GitHash        string `json:"git_hash"`
 	StartTimestamp int64  `json:"start_timestamp"`
+	Loaded         bool   `json:"loaded"`


Prefer RegionLoaded

okJiang · 2024-11-06T06:39:40Z

pkg/storage/storage.go

+	regionLoadedFromDefault bool
+	regionLoadedFromStorage bool


How about using string?

regionLoadedFrom: "", "etcd", "leveldb"

okJiang · 2024-11-06T06:43:53Z

pkg/storage/storage.go

 	if atomic.LoadInt32(&ps.useRegionStorage) == 0 {
-		return ps.Storage.LoadRegions(ctx, f)
+		err := ps.Storage.LoadRegions(ctx, f)
+		if err == nil {
+			ps.regionLoadedFromDefault = true
+		}
+		return err
 	}

-	ps.mu.Lock()
-	defer ps.mu.Unlock()
-	if !ps.regionLoaded {
+	if !ps.regionLoadedFromStorage {
 		if err := ps.regionStorage.LoadRegions(ctx, f); err != nil {
 			return err
 		}
-		ps.regionLoaded = true
+		ps.regionLoadedFromStorage = true
 	}
 	return nil


Seems we can use *coreStorage.LoadRegions() directly?

It doesn't check ps.regionLoadedFromStorage

Signed-off-by: lhy1024 <[email protected]>

niubell · 2024-11-12T06:24:08Z

pkg/storage/storage.go

-	useRegionStorage int32
-	regionLoaded     bool
-	mu               syncutil.Mutex
+	useRegionStorage        int32


Is that safe to switch between using or not using RegionStore in the master branch or the latest releases now?

defaultUseRegionStorage is true and only check switch when it was just elected leader.

Signed-off-by: lhy1024 <[email protected]>

rleungx · 2024-11-18T05:19:27Z

pkg/storage/storage.go

-	useRegionStorage int32
-	regionLoaded     bool
-	mu               syncutil.Mutex
+	useRegionStorageFlag int32


Signed-off-by: lhy1024 <[email protected]>

lhy1024 · 2024-11-18T05:55:29Z

/test pull-integration-realcluster-test

Tema · 2024-11-19T17:45:10Z

server/api/status.go

@@ -44,6 +45,7 @@ func (h *statusHandler) GetPDStatus(w http.ResponseWriter, _ *http.Request) {
 		GitHash:        versioninfo.PDGitHash,
 		Version:        versioninfo.PDReleaseVersion,
 		StartTimestamp: h.svr.StartTimestamp(),
+		RegionLoaded:   storage.AreRegionsLoaded(h.svr.GetStorage()),


Can we have a more general field here like "Ready" which we can extend in the future if we find more conditions under which PD can't serve?

Does PDStatus API exposed by all components of disaggregated PD?

Any chance we introduce a new API /ready which we can use across all TiDB components in the future?

Hi Tema, thanks for your advice
Q1: A general field will be helpful to expand different scenarios, but we found that currently there is only RegionLoaded is needed. If there are other situations in the future, we tend to add new fields with more accurate meanings.
Q2: Standalone PD or Scheduling Service will expose this API(especially the RegionLoaded field), others such as TSO Service don't.
Q3: IMO, it's good we can use the same interface to query service status, but the fields may be different. The benefits of unification may not be obvious, but it is indeed a better way.

Generally you want to decouple data plane (pd) from control plane (tidb-operator, tiUp), so that if you make changes to one, you don't have to upgrade other and it can start leveraging any improvements right away. That is why usually the common convention is to use /ready across all components. This way the control-plane can have a common logic for rolling restart of all components, without need to necessarily overcustomize each of them. This is not just about PD microservices but many other components of TiDB cluster (tidb, dm, cdc, tikv). Some of them still require additional customization, but it would be nice to keep it to the minimum. Did you talk to tidb-operator team? Don't they want to have a /ready across all components. I've herd there is a work on tidb-operator v2 or something like that. This might be a good opportunity to standartize protocol between control plane and data place as once it is out there, it is hard to change.

PTAL @csuzhangxc

I agree with Tema, for TiDB Operator, it's better to use a general API (like /ready) to check the status in most cases. If this API can't meet some other special cases in the future, then we can consider to add/use some other APIs.

For this RegionLoaded case, in fact, TiDB Operator just wants to know when it can restart the next instance safely.

api: support to query whether pd has loaded region

c20a4f9

Signed-off-by: lhy1024 <[email protected]>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. labels Oct 28, 2024

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 28, 2024

rleungx reviewed Oct 29, 2024

View reviewed changes

fix lint

db574ec

Signed-off-by: lhy1024 <[email protected]>

lhy1024 commented Oct 30, 2024

View reviewed changes

rleungx reviewed Oct 30, 2024

View reviewed changes

lhy1024 added 2 commits November 1, 2024 11:10

address comments

64f12cc

Signed-off-by: lhy1024 <[email protected]>

address comments

0c50bef

Signed-off-by: lhy1024 <[email protected]>

okJiang reviewed Nov 6, 2024

View reviewed changes

lhy1024 added 2 commits November 11, 2024 19:28

test compatibility

9c9e931

Signed-off-by: lhy1024 <[email protected]>

address comments

bc7428b

Signed-off-by: lhy1024 <[email protected]>

niubell reviewed Nov 12, 2024

View reviewed changes

refactor

df4ac32

Signed-off-by: lhy1024 <[email protected]>

rleungx reviewed Nov 18, 2024

View reviewed changes

pkg/storage/storage.go Outdated

useRegionStorage int32

regionLoaded bool

mu syncutil.Mutex

useRegionStorageFlag int32

Copy link

Member

rleungx Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use bool?

address comments

d0d6875

Signed-off-by: lhy1024 <[email protected]>

lhy1024 requested review from rleungx, okJiang and JmPotato November 19, 2024 02:29

Tema reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: support to query whether pd has loaded region #8749

api: support to query whether pd has loaded region #8749

lhy1024 commented Oct 28, 2024

ti-chi-bot bot commented Oct 28, 2024

codecov bot commented Oct 28, 2024 •

edited

Loading

rleungx Oct 29, 2024

lhy1024 Oct 29, 2024

okJiang Nov 6, 2024

lhy1024 Nov 7, 2024

JmPotato Nov 18, 2024

lhy1024 Oct 30, 2024

rleungx Oct 30, 2024

lhy1024 Oct 30, 2024

rleungx Oct 30, 2024

lhy1024 Oct 30, 2024

rleungx Oct 30, 2024

lhy1024 Oct 30, 2024

rleungx Oct 30, 2024

okJiang Nov 6, 2024

okJiang Nov 6, 2024

okJiang Nov 6, 2024

lhy1024 Nov 7, 2024

niubell Nov 12, 2024

lhy1024 Nov 12, 2024

rleungx Nov 18, 2024

lhy1024 commented Nov 18, 2024

Tema Nov 19, 2024

niubell Nov 20, 2024

Tema Nov 20, 2024

niubell Nov 21, 2024

csuzhangxc Nov 21, 2024

		@@ -39,11 +40,13 @@ func newStatusHandler(svr server.Server, rd render.Render) *statusHandler {
		// @Success 200 {object} versioninfo.Status
		// @Router /status [get]

api: support to query whether pd has loaded region #8749

Are you sure you want to change the base?

api: support to query whether pd has loaded region #8749

Conversation

lhy1024 commented Oct 28, 2024

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Oct 28, 2024

codecov bot commented Oct 28, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhy1024 commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 28, 2024 •

edited

Loading